Bug 7875 - AskDNS plugin does not correctly handle CNAMEs leading to TXTs
Summary: AskDNS plugin does not correctly handle CNAMEs leading to TXTs
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Plugins (show other bugs)
Version: 3.4.4
Hardware: PC All
: P2 normal
Target Milestone: 3.4.5
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-15 17:16 UTC by Robert L Mathews
Modified: 2021-03-09 20:15 UTC (History)
4 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Robert L Mathews 2020-12-15 17:16:03 UTC 
Summary: When the ASkDNS plugin requests a TXT record but gets back both a CNAME and a TXT, it doesn't ignore the CNAME answer in the result, which then makes it not handle the TXT answer properly.

Detail: I have a rule that looks like this:

askdns __DMARC_POLICY_REJECT _dmarc._AUTHORDOMAIN_ TXT /^v=DMARC1;.*\bp=reject;/

I noticed this rule did not match a lookup for "_dmarc.dhl.com", even though it should:

$ dig _dmarc.dhl.com TXT
...
;; ANSWER SECTION:
_dmarc.dhl.com. 269 IN CNAME reject.valimail.dmarc.dhl.com.
reject.valimail.dmarc.dhl.com. 340 IN TXT "v=DMARC1; p=reject; fo=0; rua=mailto:dmarc-reports@dhl.com,mailto:dmarc_agg@vali.email;"

Some debugging suggests this is because of the CNAME in the answer. The loop in lines 567-640 of AskDNS.pm runs once for the CNAME answer and a second time for the TXT answer. Line 602 of that block looks like:

 next  if !defined $qtype || $query_type ne $qtype;

In this case, $qtype is the qtype from the original DNS *request* that was made, which is always "TXT". And $query_type is the type from the SpamAssassin rule, which is also always "TXT". So the code will always check "TXT ne TXT", find it's false, and the "next" will never trigger.

The code should be checking one of these against the qtype of each DNS *reply*, so it calls "next" to skip the loop when it sees the CNAME. Changing that line to this fixes it:

 next  if !defined $qtype || $rr_type ne $qtype;
Comment 1 Robert L Mathews 2020-12-15 18:04:01 UTC 
As a followup to this, this change is more correct for line 602:

 next  if !defined $qtype || (defined $rr_type && $rr_type ne $qtype);

Otherwise what I originally suggested can generate "uninitialized value $rr_type in string" warnings in some cases.
Comment 2 Michael Grant 2021-02-28 16:17:03 UTC 
I also seem to have this problem if I use a DNAME record like this in db.local:

sendgrid-id     IN DNAME        sendgrid-id.LICENSEKEY.invaluement.com.
sendgrid-efd    IN DNAME        sendgrid-efd.LICENSEKEY.invaluement.com.

and in Esp.cf:

ifplugin Mail::SpamAssassin::Plugin::AskDNS

  askdns   RBL_SENDGRID_ID _SENDGRIDID_.sendgrid-id.localhost A 127.0.0.2
#  askdns   RBL_SENDGRID_ID _SENDGRIDID_.sendgrid-id.<LICENSEKEY>.invaluement.com A 127.0.0.2
  describe RBL_SENDGRID_ID Sendgrid Id blacklist
  tflags   RBL_SENDGRID_ID net

  askdns   RBL_SENDGRID_DOM _SENDGRIDDOM_.sendgrid-efd.localhost A 127.0.0.2
#  askdns   RBL_SENDGRID_DOM _SENDGRIDDOM_.sendgrid-efd.<LICENSEKEY>.invaluement.com A 127.0.0.2
  describe RBL_SENDGRID_DOM Sendgrid domain blacklist
  tflags   RBL_SENDGRID_DOM net

meta       RBL_SENDGRID (RBL_SENDGRID_ID || RBL_SENDGRID_DOM)
describe   RBL_SENDGRID Invaluement Sendgrid blacklist
score      RBL_SENDGRID 7.5

endif # Mail::SpamAssassin::Plugin::AskDNS

If I uncomment the dbg message on line 594, I absolutely see the 127.0.0.2 response, but it doesn't get picked up in the loop below.
Comment 3 Henrik Krohns 2021-03-07 21:51:51 UTC 
The whole $qtype comparison completely unnecessary and breaks stuff, RR comparison is done later here in if:

                 !grep($_ eq 'ANY' || $_ eq $rr_type, @$answer_types_ref) ) {
          # skip remaining tests on wrong RR type

.. which allows for ANY / multiple RR type queries to work too.

I spend some time looking through the code carefully, this should be safe to remove for 3.4..

$pms->{askdns_map_dnskey_to_rules}{$dnskey}[$j++] = undef;

It fixes this bug and the multiple response bug discussed in Bug 7777.

Similar fixes committed to trunk also:

Sending        spamassassin-3.4/lib/Mail/SpamAssassin/Plugin/AskDNS.pm
Sending        trunk/lib/Mail/SpamAssassin/Plugin/AskDNS.pm
Transmitting file data ..done
Committing transaction...
Committed revision 1887305.
Comment 4 Henrik Krohns 2021-03-07 21:58:46 UTC 
Duh, should allow multiple hits..

Sending        spamassassin-3.4/lib/Mail/SpamAssassin/Plugin/AskDNS.pm
Transmitting file data .done
Committing transaction...
Committed revision 1887306.